hey @nelsonic, I notice that the Github Checks didn't pass, is there something on my end that I need to fix?

Hey @panoramix360 .
Thanks for the PR! Run mix format to fix the Build and Test workflow. We have it setup so the code is correctly formatted before being pushed into the repo.
Thanks!

thanks @LuchoTurtle!

Done!

@nelsonic we're getting an error with the deploy review app script.

https://github.com/dwyl/mvp/actions/runs/5488807164/jobs/10002305167

Review App Script
Error: No access token available. Please login with 'flyctl auth login'

Have you stumbled upon this error before?

We need to set FLY_ACCESS_TOKEN in order for this to work.

https://fly.io/docs/flyctl/integrating/

@LuchoTurtle, is this something that I should do on the PR?

@panoramix360 the issue is on our end, so please just give us some time so get it sorted and we'll get right back to ya with this PR and get it merged :).

Meanwhile, we're getting this warning with the function you've added.

clauses with the same name and arity (number of arguments) should be grouped together, "def handle_event/3" was previously defined (lib/app_web/live/app_live.ex:45)

I'll create a review and request a small change from you :)

@panoramix360 I think the CI doesn't run because you created a fork instead of a branch. 💭
We need to fix that on our side as @LuchoTurtle says. 🔧
You have an invite for write access to the repo for any future contributions. ✍️

I checked out @panoramix360 branch:

git clone [email protected]:panoramix360/mvp.git && cd mvp
git checkout 'feature/add-task-with-tag-when-enter'
cp ../.env .
source .env
mix deps.get
mix c

Got:

Finished in 1.2 seconds (1.1s async, 0.1s sync)
118 tests, 0 failures

Randomized with seed 640070
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/api/item.ex                               218       56        0
100.0% lib/api/tag.ex                                101       24        0
100.0% lib/api/timer.ex                              152       40        0
100.0% lib/app/color.ex                               90        1        0
100.0% lib/app/item.ex                               377       53        0
100.0% lib/app/item_tag.ex                            12        1        0
100.0% lib/app/tag.ex                                108       18        0
100.0% lib/app/timer.ex                              481       90        0
100.0% lib/app_web/controllers/auth_controller.       26        4        0
100.0% lib/app_web/controllers/init_controller.       41        6        0
100.0% lib/app_web/controllers/tag_controller.e       77       25        0
 94.6% lib/app_web/live/app_live.ex                  441      130        7
100.0% lib/app_web/live/stats_live.ex                 77       21        0
100.0% lib/app_web/router.ex                          49        9        0
100.0% lib/app_web/views/error_view.ex                59       12        0
  0.0% lib/app_web/views/profile_view.ex               3        0        0
  0.0% lib/app_web/views/tag_view.ex                   3        0        0
[TOTAL]  98.6%
----------------
Generating report...

i.e. test coverage decreased by 1.4% due to the new code.

Reviewing the HTML coverage report we see that the lines that are not covered by tests are exactly the lines that are being added by this PR:

Sadly, CI doesn't pass because @panoramix360 created a fork.
This is a problem with how GitHub handles Environment Variables. 🤦‍♂️

The only way we can make the CI pass is to make the AUTH_API_KEY public. 🔓
Or for @panoramix360 to re-create these changes as a branch on the @dwyl source version.
I would prefer to resolve the Environment Variables once and for all to avoid wasting contributor time in the future.

The CI error is in the Build and Test phase: https://github.com/dwyl/mvp/actions/runs/5514171282/jobs/10060694853?pr=389#step:8:66

This is because GitHub does not give access to AUTH_API_KEY to PRs based on Forks.
This is a super annoying feature of GitHub CI we cannot circumvent. 😢

To write a test for this new functionality we need to emit a keydown event:
https://hexdocs.pm/phoenix_live_view/Phoenix.LiveViewTest.html#render_keydown/1
I'm happy to add this test if you're short on time @panoramix360

As for the failing Review App Job: https://github.com/dwyl/mvp/actions/runs/5488807164/jobs/10002305167#step:5:20

This is exactly the same issue described above, GitHub is blocking access to the FLY_API_TOKEN environment variable because the PR was created from a fork.

Once this is merged into main the deploy to fly.io will work fine as it has in the past.

Sadly, we still have the regression that we cannot add tags dynamically as outlined in: #221 (comment)

But if I manually add a couple of tags on my localhost:

And then test the feature that @panoramix360 is proposing:

I cannot get the desired behaviour described by @iteles in #308 (comment) 💭

@iteles could you please help us with the requirements so that we can review this PR ASAP. 🙏

@panoramix360 meanwhile if you can add a test for the Enter keypress event I will git pull from your branch. 🙏

hey! Thank you for inviting me to the repo, if you prefer I can create the branch on the repo and delete the fork, but as you mentioned, it's good to have that fixed for future contributions.

I can create the tests, no problem, sorry for not creating it earlier, I didn't see that the project had tests.

Regarding the behavior, what happened when you pressed enter? It didn't work?

hey @nelsonic! do you have any tips on how to test it? I'm new to Phoenix LiveView and I'm thinking of using this one to test it:
https://hexdocs.pm/phoenix_live_view/Phoenix.LiveViewTest.html#render_keydown/1

If you have any ideas or directions to it, it'd be helpful!

EDIT: It's tested now, scroll through the next comment

I'm trying to do something like that:

• Create a new tag on the database (Tag.create_tag)
• Then I use the render_keydown to fire the Enter event
• Then I use the render_submit to create a new item on the database
• Then check if the item is created with the first tag

This is my code:

test "select tag when enter pressed", %{conn: conn} do
    {:ok, view, html} = live(conn, "/")

    {:ok, _tag1} =
      Tag.create_tag(%{person_id: 0, text: "tag1", color: "#FCA5A5"})

    {:ok, _tag2} =
      Tag.create_tag(%{person_id: 0, text: "enter_tag", color: "#FCA5A5"})

    assert render_keydown(view, "add-first-tag", %{
             "key" => "Enter",
             "value" => "enter_tag"
           })

    assert render_submit(view, :create, %{text: "tag enter pressed"})

    IO.inspect(Tag.list_person_tags(0))
    IO.inspect(Item.get_item!(2).tags)
  end

(Ignore the IO.inspect, is just for checking)

It seems that the render_keydown is not finding any tags created, which is strange since I was creating before it.

I believe I'm missing something

UPDATE:

I managed to test it right now :D

@nelsonic review it when you can! thank you for your link, it was helpful!

The test coverage is now 99.8%, feel free to let me know to add more things if needed.

Hey @panoramix360

Awesome job on proactively working on this, it's mighty appreciated! I've pulled the remote from your forked repo and ran your branch locally and it seems to work as you've described.

I've seen you've mentioned it only toggles the tag on top of the list. Although it would make more sense to get the first tag not selected, I'm aware that you've tried to make it simple.

When I run the tests mix c, they seem to pass as well:

Finished in 1.1 seconds (1.0s async, 0.08s sync)
121 tests, 0 failures

Randomized with seed 879703
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/api/item.ex                               218       56        0
100.0% lib/api/tag.ex                                101       24        0
100.0% lib/api/timer.ex                              152       40        0
100.0% lib/app/color.ex                               90        1        0
100.0% lib/app/item.ex                               377       53        0
100.0% lib/app/item_tag.ex                            12        1        0
100.0% lib/app/tag.ex                                108       18        0
100.0% lib/app/timer.ex                              481       90        0
100.0% lib/app_web/controllers/auth_controller.       26        4        0
100.0% lib/app_web/controllers/init_controller.       41        6        0
100.0% lib/app_web/controllers/tag_controller.e       77       25        0
100.0% lib/app_web/live/app_live.ex                  441      130        0
100.0% lib/app_web/live/stats_live.ex                 77       21        0
100.0% lib/app_web/router.ex                          49        9        0
100.0% lib/app_web/views/error_view.ex                59       12        0
  0.0% lib/app_web/views/profile_view.ex               3        0        0
  0.0% lib/app_web/views/tag_view.ex                   3        0        0
[TOTAL] 100.0%

Thanks for getting the test coverage back to 100%. I'm glad you've worked more on them (saw your latest commits), the assertions make more sense now and actually test if the tag is selected.

I'm going to make a small comment on the review shortly :)

No worries! I'll look through the next issue to complete, if you have any suggestions let me know!